Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Various bid adapters: Currency config cleanup #12102

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

mkomorski
Copy link
Collaborator

Type of change

  • Bugfix

  • Feature

  • New bidder adapter

  • Updated bidder adapter

  • Code style update (formatting, local variables)

  • Refactoring (no functional changes, no api changes)

  • Build related changes

  • CI related changes

  • Does this change affect user-facing APIs or examples documented on http://prebid.org?

  • Other

Description of change

Other information

#10656

@mkomorski mkomorski changed the title 10656 currency config cleanup Various bid adapters: Currency config cleanup #12051 Aug 7, 2024
@mkomorski mkomorski changed the title Various bid adapters: Currency config cleanup #12051 Various bid adapters: Currency config cleanup Aug 7, 2024
Copy link

github-actions bot commented Aug 7, 2024

Whoa there, partner! 🌵🤠 We wrangled some duplicated code in your PR:

Reducing code duplication by importing common functions from a library not only makes our code cleaner but also easier to maintain. Please move the common code from both files into a library and import it in each. We hate that we have to mention this, however, commits designed to hide from this utility by renaming variables or reordering an object are poor conduct. We will not look upon them kindly! Keep up the great work! 🚀


function enrichFPDHook(next, fpd) {
return next(fpd.then(ortb2 => {
deepSetValue(ortb2, 'ext.cur', [adServerCurrency]);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ext.prebid.adserverCurrency is a better name I think - and it doesn't need to be an array

(ext.cur looks like just a misplaced cur. adserverCurrency makes it clearer what it is, and prebid makes it less likely to clash)

return (bidderRequest?.ortb2?.ext?.cur || [])[0];
}

export function setAdditionalData(obj, key, value) {
Copy link
Collaborator

@dgirardi dgirardi Aug 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if these are only used for adserverCurrency, they'd be better as set/getAdServerCurrency (they are in a currency library after all, unlikely/unsuitable to be used for anything else).

But if they are only used for one adapter, I don't think they're needed at all - the adapter itself can maintain the map.

Copy link

Tread carefully! This PR adds 1 linter error (possibly disabled through directives):

  • libraries/ortb2Utils/currency.js (+1 error)

Copy link

Whoa there, partner! 🌵🤠 We wrangled some duplicated code in your PR:

Reducing code duplication by importing common functions from a library not only makes our code cleaner but also easier to maintain. Please move the common code from both files into a library and import it in each. We hate that we have to mention this, however, commits designed to hide from this utility by renaming variables or reordering an object are poor conduct. We will not look upon them kindly! Keep up the great work! 🚀

@mkomorski mkomorski force-pushed the 10656-currency-config-cleanup branch from 0665ae1 to 1ccb05e Compare August 14, 2024 10:34
Copy link

Whoa there, partner! 🌵🤠 We wrangled some duplicated code in your PR:

Reducing code duplication by importing common functions from a library not only makes our code cleaner but also easier to maintain. Please move the common code from both files into a library and import it in each. We hate that we have to mention this, however, commits designed to hide from this utility by renaming variables or reordering an object are poor conduct. We will not look upon them kindly! Keep up the great work! 🚀

Copy link

Tread carefully! This PR adds 1 linter error (possibly disabled through directives):

  • libraries/ortb2Utils/currency.js (+1 error)

@mkomorski mkomorski force-pushed the 10656-currency-config-cleanup branch from 1ccb05e to cb790fb Compare August 14, 2024 12:32
Copy link

Whoa there, partner! 🌵🤠 We wrangled some duplicated code in your PR:

Reducing code duplication by importing common functions from a library not only makes our code cleaner but also easier to maintain. Please move the common code from both files into a library and import it in each. We hate that we have to mention this, however, commits designed to hide from this utility by renaming variables or reordering an object are poor conduct. We will not look upon them kindly! Keep up the great work! 🚀

Copy link

Whoa there, partner! 🌵🤠 We wrangled some duplicated code in your PR:

Reducing code duplication by importing common functions from a library not only makes our code cleaner but also easier to maintain. Please move the common code from both files into a library and import it in each. We hate that we have to mention this, however, commits designed to hide from this utility by renaming variables or reordering an object are poor conduct. We will not look upon them kindly! Keep up the great work! 🚀

Copy link

Whoa there, partner! 🌵🤠 We wrangled some duplicated code in your PR:

Reducing code duplication by importing common functions from a library not only makes our code cleaner but also easier to maintain. Please move the common code from both files into a library and import it in each. We hate that we have to mention this, however, commits designed to hide from this utility by renaming variables or reordering an object are poor conduct. We will not look upon them kindly! Keep up the great work! 🚀

@dgirardi dgirardi added the needs 2nd review Core module updates require two approvals from the core team label Aug 15, 2024
Copy link

Whoa there, partner! 🌵🤠 We wrangled some duplicated code in your PR:

Reducing code duplication by importing common functions from a library not only makes our code cleaner but also easier to maintain. Please move the common code from both files into a library and import it in each. We hate that we have to mention this, however, commits designed to hide from this utility by renaming variables or reordering an object are poor conduct. We will not look upon them kindly! Keep up the great work! 🚀

Copy link

Tread carefully! This PR adds 4 linter errors (possibly disabled through directives):

  • modules/admaticBidAdapter.js (+4 errors)

Copy link

Whoa there, partner! 🌵🤠 We wrangled some duplicated code in your PR:

Reducing code duplication by importing common functions from a library not only makes our code cleaner but also easier to maintain. Please move the common code from both files into a library and import it in each. We hate that we have to mention this, however, commits designed to hide from this utility by renaming variables or reordering an object are poor conduct. We will not look upon them kindly! Keep up the great work! 🚀

Copy link

Tread carefully! This PR adds 1 linter error (possibly disabled through directives):

  • modules/admaticBidAdapter.js (+1 error)

@ChrisHuie ChrisHuie self-requested a review October 25, 2024 13:55
@ChrisHuie ChrisHuie self-assigned this Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance needs 2nd review Core module updates require two approvals from the core team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants